Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: remove unused assignment #35642

Merged
merged 1 commit into from
Oct 16, 2020
Merged

fs: remove unused assignment #35642

merged 1 commit into from
Oct 16, 2020
+1 −1

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 14, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 14, 2020
@Trott
Copy link
Member Author

Trott commented Oct 14, 2020 •
edited
Loading

@iansu @bcoe @cjihrig This can totally wait until after the push to get everything set up for 15.0 and so on--I know there's a lot going on right now--but I was wondering about taking this trivial change a little further and removing the options validation entirely in cases like this when options.recursive is not set and the validated options object is not subsequently used. I might rename validateRmdirOptions() to validateRecursiveRmdirOptions() to make it clear that the only options validated are ones used with the recursive setting (recursive, maxRetries, and retryDelay) and skip it here and in other places where the validated options aren't used.

Another alternative is to keep the validation but warn the user when those other two options are set and recursive is not.

Mentioning that in case I'm missing/forgetting something that makes that A Bad Idea.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@iansu
Copy link
Contributor

iansu commented Oct 14, 2020

@Trott I think warning when setting either maxRetries or retryDelay without recursive sounds like a good idea. That seems preferable to skipping validation entirely when recursive is not set.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/33666/

@Trott
fs: remove unused assignment
7f25fe8 
PR-URL: nodejs#35642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 16, 2020

Landed in 7f25fe8

@Trott Trott merged commit 7f25fe8 into nodejs:master Oct 16, 2020
@Trott Trott deleted the validate-recursive branch October 16, 2020 13:32
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
@Trott @BethGriggs
fs: remove unused assignment
5f0c814 
PR-URL: #35642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@watilde watilde watilde approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @nodejs-github-bot @iansu @jasnell @watilde @cjihrig @joyeecheung